Claude finished @wezell's task in 1m 9s —— View job

Code Review

• Read diff and full file
• Analyze for bugs, edge cases, design issues

Three issues worth addressing before merge:

1. Config inconsistency — overlapTokens bypasses per-host configuration

splitAtTokens is read via embeddingsAPI.config.getConfigInteger(AppKeys.EMBEDDINGS_SPLIT_AT_TOKENS) (per-app/host config), but overlapTokens uses Config.getIntProperty(...) — a global JVM-level property. This means the overlap can't be tuned per dotCMS site the way the split threshold can. It should either be added to AppKeys and read through the same embeddingsAPI.config mechanism, or at minimum documented as intentionally global.

2. Missing guard: overlapTokens >= splitAtTokens causes degenerate behavior

EmbeddingsRunner.java:94

while (overlapStart > 0 && overlapCount < overlapTokens) {

If someone sets overlapTokens close to or above splitAtTokens, the overlap carried into the next chunk will immediately trigger another split on the very first sentence added, producing near-empty or near-duplicate chunks in a tight loop. There's no validation that overlapTokens < splitAtTokens. A guard + log warning at line 65 would prevent misconfiguration.

3. A single oversized sentence can silently blow past the overlap target

The while loop exits when overlapCount >= overlapTokens OR when overlapStart == 0. If the very first sentence walked back has 300 tokens, the overlap becomes 300 tokens — 6× the configured limit. For short-sentence prose this is fine, but for structured content (tables, code blocks treated as one "sentence" by BreakIterator) the overlap can approach splitAtTokens, making the next chunk nearly a full duplicate of the previous one. Worth at minimum a Logger.debug when overlapCount > overlapTokens * 2.

One subtlety that's correct but fragile:

hasUnsavedContent (EmbeddingsRunner.java:75, 86, 107) exists to prevent double-saving the overlap sentences after the final split. It's correct, but sentences serves dual purpose (accumulation buffer + overlap buffer), which a future maintainer could break by removing the flag after seeing !sentences.isEmpty() as sufficient. A comment explaining why the flag exists alongside sentences (i.e., "sentences may hold overlap-only content that was already saved") would prevent that regression.

No functional bugs found — the core logic (backward traversal for overlap, clearing lists after save, totalTokens = overlapCount for continuity) is sound. The issues above are defensive-correctness gaps, not correctness failures.